Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor package infrastructure/kubernetes #1259

Merged
merged 4 commits into from
Apr 20, 2023

Conversation

zirain
Copy link
Member

@zirain zirain commented Apr 6, 2023

refactor package to reuse code :

  1. use Applier to create/update/delete Kubernetes resources
  2. use ResourceRender to render resource for Proxy/RateLimit
  3. move common functions to utils package

cc @arkodg

@zirain zirain requested a review from a team as a code owner April 6, 2023 08:04
@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Merging #1259 (20274eb) into main (5d360cb) will increase coverage by 0.22%.
The diff coverage is 88.96%.

@@            Coverage Diff             @@
##             main    #1259      +/-   ##
==========================================
+ Coverage   61.81%   62.03%   +0.22%     
==========================================
  Files          85       76       -9     
  Lines       10854    10701     -153     
==========================================
- Hits         6709     6638      -71     
+ Misses       3700     3624      -76     
+ Partials      445      439       -6     
Impacted Files Coverage Δ
internal/xds/translator/runner/runner.go 70.00% <0.00%> (ø)
internal/infrastructure/kubernetes/infra.go 68.42% <66.29%> (-31.58%) ⬇️
...frastructure/kubernetes/proxy/resource_provider.go 88.76% <86.40%> (ø)
...tructure/kubernetes/ratelimit/resource_provider.go 97.26% <97.26%> (ø)
internal/cmd/egctl/translate.go 78.67% <100.00%> (ø)
internal/infrastructure/kubernetes/proxy/utils.go 100.00% <100.00%> (ø)
internal/infrastructure/kubernetes/proxy_infra.go 100.00% <100.00%> (+56.75%) ⬆️
.../infrastructure/kubernetes/ratelimit/deployment.go 100.00% <100.00%> (ø)
...ernal/infrastructure/kubernetes/ratelimit/utils.go 100.00% <100.00%> (ø)
...ernal/infrastructure/kubernetes/ratelimit_infra.go 100.00% <100.00%> (+100.00%) ⬆️
... and 1 more

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@zirain zirain force-pushed the refactor-component branch 10 times, most recently from 59ee7b3 to 0255bad Compare April 11, 2023 03:59
@Xunzhuo Xunzhuo requested review from a team, kflynn, zhaohuabing and chauhanshubham and removed request for a team April 11, 2023 07:04
@zirain zirain added this to the 0.5.0-rc1 milestone Apr 12, 2023
return nil
}

func (i *Instance) DeleteConfigMap(ctx context.Context, cm *corev1.ConfigMap) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can just pass the ns and name to delete a specific resource, or we can use a single function to delete all kinds of resources.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this PR is too large to review, let's do more refactor later.
Applier absolute can be more generic.

Signed-off-by: hejianpeng <[email protected]>
@arkodg
Copy link
Contributor

arkodg commented Apr 17, 2023

@qicz can you also help review this PR since you've done some refactoring in this area in the past, thanks in advance !

@qicz
Copy link
Member

qicz commented Apr 17, 2023

@qicz can you also help review this PR since you've done some refactoring in this area in the past, thanks in advance !

No problem. in fact, i have already reviewed a few days ago without.note, i will do this again.

// expectedProxyDeployment returns the expected Deployment based on the provided infra.
func (i *Infra) expectedProxyDeployment(infra *ir.Infra) (*appsv1.Deployment, error) {
type ResourceRender struct {
infra *ir.Infra
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using ir.ProxyInfra directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it's too wide to use ProxyInfra here for now, change it when necessary in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

address follow up here: #1330 (comment)

internal/infrastructure/kubernetes/utils/utils.go Outdated Show resolved Hide resolved
internal/infrastructure/kubernetes/utils/utils.go Outdated Show resolved Hide resolved
Signed-off-by: hejianpeng <[email protected]>
Copy link
Member

@qicz qicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I will follow up for this refactor in another pr. issue #1330

@zirain zirain merged commit e72598b into envoyproxy:main Apr 20, 2023
@zirain zirain deleted the refactor-component branch April 20, 2023 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants